-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[CD] Adds python docker pipeline #16547
[CD] Adds python docker pipeline #16547
Conversation
Test safe docker run tweaks Fix up
02a364f
to
5e82abe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!! Thanks a ton. Ready to merge whenever you are ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a few questions in the review.
I looked at the shell output from the push stage and was hoping to see a link to the asset. Where'd they go? Seems like the only references are abstracted to whatever the secrets config is...
This is great btw.
cd/utils/mxnet_base_image.sh
Outdated
cu90*) | ||
echo "nvidia/cuda:9.0-cudnn7-runtime-ubuntu16.04" | ||
;; | ||
cu91*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always pick on this one... but isn't it not supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll remove
import pprint | ||
|
||
|
||
class Cleanup: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So all of this is in the new docker client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - it all moved to the "safe_docker_run.py" script
@@ -305,61 +246,7 @@ def container_run(platform: str, | |||
{'bind': '/work/ccache', 'mode': 'rw'}, | |||
}, | |||
environment=environment) | |||
try: | |||
logging.info("Started container: %s", trim_container_id(container.id)) | |||
# Race condition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is no longer an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still a thing - the signal blocking has moved to the "safe_docker_run.py" script ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move these comments in there for clarity though - good catch!
@aaronmarkham you can see the push statement - it's not possible to put a link to the image (the best that can be done is a link to the repository. I'll add a message with the docker pull command to run to get it. Let me know if you would prefer something else =) |
Just notice this: |
@aaronmarkham nope, that's bad. I've rotated the credentials. I've checked the DockerHub account and nothing has changed there. Thanks for catching this! I've updated the logging configuration and I'm running a job to double check. |
Okay, looks good now. I'm.glad you took the time to have a look! Thank you!! |
@aaronmarkham if you are cool with everything, please merge when you get a chance =D |
Again, thanks a lot for this contribution! Can you let us (and dev@) know once the images are being published? |
Description
Updates CD pipeline to post python docker images. Addressed comments from closed PR #16435
@marcoabreu I've decided I would like to finish this. I've addressed your previous comments.
Example CI-dev run
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.